Removing breaking change for TaskOptions #446
Conversation
|
@sophiatev thanks for this PR! Looking at the code history, I think there might be a more correct change. The first constructor was originally the only constructor, so I think a safer fix would be to just change the second constructor (which is the new one) to make both parameters required. Your current fix solves the ambiguity problem, but I'm not 100% certain that it will solve the binary compatibility problem. Also adding @YunchuWang for visibility since it was #426 that introduced this change. |
@YunchuWang is there ever a situation where you would want to allow just specifying a Also adding @jviau since he suggested the initial fix and has a much better understanding of this binary compatibility stuff than I do. |
There was no binary compatibility problem with the original change. Only a new ambiguity issue at compilation time. The proposed changes resolve the ambiguity problem and do not affect the binary compatibility (so all good there). With that said, you would prefer the 2nd ctor to have optional params removed that is fine as well. Its a question of, do you want |
| startOptions.Version.Should().BeNull(); | ||
| startOptions.InstanceId.Should().BeNull(); | ||
| startOptions.StartAt.Should().BeNull(); | ||
| startOptions.Tags.Should().BeEmpty(); |
There was a problem hiding this comment.
Weird that tags defaults to empty here but defaults to null in the *Options classes. Oh well - not something to deal with in this PR.
A breaking change was introduced in
TaskOptionswhere there are now two constructors which both have all optional parameters. Now creating a newTaskOption()with no arguments is ambiguous between the two constructors. One constructor was changed to have its parameter be non-optional, whereas the other constructor remains as having all optional parameters. This should make creating a newTaskOption()unambiguous (it will correspond to the second constructor with two optional parameters).